Keep the test results of a failed CI job - #216
Merged
Merged
Conversation
The test jobs upload their coverage data even when a step failed, because that is the run the data is most useful for. The test results of that same failure were not kept, so the only trace of a red matrix cell was the job log. The whole surefire-reports directory is uploaded rather than *.xml: a fork that crashed or hit forkedProcessTimeoutInSeconds leaves a .dump or .dumpstream behind and frequently no XML at all, which is exactly the case worth keeping.
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A test job that goes red keeps its coverage data but threw away its test results. The whole
surefire-reportsdirectory is now uploaded from every test job, so a failed matrix cell leaves something a reviewer can open.Linked issues
No linked issues
1. Problem
The three test jobs of the Maven workflow upload
target/jacoco.execwithif: !cancelled(), on the stated grounds that a failed step is exactly the run whose data is worth keeping. The test results of that same failure were not uploaded at all.The consequence is that a red matrix cell leaves only the job log. That log expires, cannot be searched across runs, and cannot be fed to anything. Nothing is at fault in Ares itself; this is a gap in what CI keeps.
2. Improvement from the user's perspective
No Improvement from the user's perspective
3. Improvement from the maintainer's perspective
A maintainer looking at a failed run can download the results of the run that failed instead of reading the log. The whole directory is uploaded rather than
*.xml, because a fork that crashed or ran intoforkedProcessTimeoutInSecondsleaves a.dump,.dumpstreamor.txtbehind and often no XML at all, which is precisely the case the artefact exists for.4. Testing manual
Prerequisites
Steps
Not reproducible from an exercise. This changes a CI workflow only, so a reviewer verifies it from a workflow run.
Expected result
Step 2 lists
test-results-unit-tests,test-results-core-integration-testsand onetest-results-<slug>per matrix cell, alongside the existingjacoco-exec-*artefacts. Each contains the contents oftarget/surefire-reports/.Negative case (what must still be rejected)
Nothing about what Ares blocks changes here. The workflow must still fail when a test fails; the upload runs with
if: !cancelled()and must not mask a failing step.Modes exercised
No mode-specific behaviour changed.
5. Test case coverage regarding this PR
No production Java code changed
Breaking changes and migration
No breaking changes or migration.
Checklist
Review progress